Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add to safe query_interface class #174

Merged
merged 3 commits into from
Oct 20, 2020
Merged

Conversation

rylev
Copy link
Contributor

@rylev rylev commented Oct 19, 2020

Fixes issue brought up by @MarijnS95 in #173


if ::com::sys::FAILED(hr) {
assert!(
hr == ::com::sys::E_NOINTERFACE || hr == ::com::sys::E_POINTER,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI. In theory, E_POINTER would only be returned if the second parameter is null. The caller can guarantee that this is not the case. In practice, implementations will typically AV by dereferencing the pointer without bothering to check since this is a coding error by the caller. Either way, asserting here is not advised as wonky implementations may return some non-standard result.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since only E_NOINTERFACE is expected it probably doesn't make a lot of sense to return Err(hr) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarijnS95 Hmm I'm not sure what errors would be recoverable from here? I think in general, it's just a question of whether it worked or not, but perhaps you're right that for ultimate flexibility we should be returning an HResult on error so that if some implementation returns a non-standard error, the user can inspect that...

@MarijnS95
Copy link
Contributor

MarijnS95 commented Oct 19, 2020

Thanks for re-adding this function, but it is just a copypaste of IUnknown::query_interface. Wouldn't it make more sense to Deref from a class to the underlying interface, which recursively derefs to IUnknown?

EDIT: I guess since one class can possibly inherit multiple interfaces this is more troublesome to implement easily?

@rylev
Copy link
Contributor Author

rylev commented Oct 19, 2020

@MarijnS95 Deref takes a &Self but we want to ensure that the caller has a Box<Pin<Self>>

@MarijnS95
Copy link
Contributor

@MarijnS95 Deref takes a &Self but we want to ensure that the caller has a Box<Pin<Self>>

I guess that's a fair point. In the same way that this would segfault if transmuting a Class to it's interface and/or IUnknown, then calling QueryInterface on it. Not sure why, let's stick to the current version. Thanks for fixing 👍

@rylev
Copy link
Contributor Author

rylev commented Oct 20, 2020

Going to merge for now. I created an issue for discussing whether query_interface should return a result instead of an option: #176

@rylev rylev merged commit 32fa125 into master Oct 20, 2020
@rylev rylev deleted the safe-query-interface-for-class branch October 20, 2020 12:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants